fix(hydration): route mismatches via handleError when consumer set#14757
fix(hydration): route mismatches via handleError when consumer set#14757pierluigilenoci wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughHydration mismatch reporting is routed through Vue's error pipeline: add ErrorCodes.HYDRATION_MISMATCH, implement logMismatchError(instance) with a dedupe guard and resetHydrationMismatchState(), update mismatch call sites to pass component context, and add tests verifying handlers receive the error. ChangesHydration mismatch handling
Sequence DiagramsequenceDiagram
participant HydrationSystem
participant logMismatchError
participant handleError
participant AppErrorHandler
participant ComponentCapture
HydrationSystem->>logMismatchError: detect mismatch (parentComponent)
logMismatchError->>logMismatchError: check hasLoggedMismatchError
alt not logged
logMismatchError->>logMismatchError: set hasLoggedMismatchError = true
logMismatchError->>handleError: handleError(Error("Hydration completed but contains mismatches."), instance, ErrorCodes.HYDRATION_MISMATCH)
handleError->>AppErrorHandler: invoke if configured
handleError->>ComponentCapture: deliver to captured ancestor handlers (onErrorCaptured)
else already logged
logMismatchError->>logMismatchError: skip duplicate emission
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/runtime-core/src/hydration.ts`:
- Around line 68-92: The current call to handleError(...) always routes through
logError() and emits extra dev warnings; change the logic so handleError is only
invoked when there is an actual consumer (check
instance.appContext.config.errorHandler or hasErrorCaptured(instance)), and when
no consumer exists fall back to directly calling console.error(new
Error('Hydration completed but contains mismatches.')) instead of handleError;
update both the __TEST__ branch and the default branch to use this gating around
handleError and preserve the existing ErrorCodes.HYDRATION_MISMATCH constant
when routing to handleError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4511270-f4c7-4adc-aa7e-d07fa84ea855
📒 Files selected for processing (3)
packages/runtime-core/__tests__/hydration.spec.tspackages/runtime-core/src/errorHandling.tspackages/runtime-core/src/hydration.ts
|
Hi — friendly ping. Is this PR still on the radar for review? Happy to rebase or make changes if needed. Thanks! |
|
I agree the observability use case is valid, but I don't think this should be merged in the current form. Routing hydration mismatch reporting through I think the safer direction is to preserve the current default behavior and only expose hydration mismatch reporting to user handlers when there is an explicit consumer, or consider an app-level / warning-handler based API instead of putting it fully into the component error boundary pipeline. |
|
Hi — friendly follow-up. CI is green and all checks pass. Would you be able to review when you get a chance? Thank you! |
5e1c9ff to
81d3a55
Compare
|
@edison1105 thanks for the careful review — fully agree. I just pushed a rework that addresses your concerns:
Without a consumer, the per-mismatch Other concerns:
Tests cover all four cases: Let me know if the gating-on-consumer shape works for you, or if you'd prefer the app-level / warning-handler API instead — happy to iterate. |
81d3a55 to
68656d1
Compare
Hydration mismatch errors were never routed through Vue's error handling pipeline, so onErrorCaptured and app.config.errorHandler could not catch them — making it hard to wire hydration mismatch reporting into observability tools (fix vuejs#13154). logMismatchError now calls handleError(..., HYDRATION_MISMATCH, false) only when an explicit consumer is present: - app.config.errorHandler is set, or - some ancestor has registered onErrorCaptured. Without a consumer, the per-mismatch warn() calls at the call sites remain the only output, matching the prior default behavior — no 'Unhandled error during execution of hydration' warning is emitted for SSR apps that have not opted in. Adds ErrorCodes.HYDRATION_MISMATCH plus its 'hydration' info label, passes the parent component instance to logMismatchError() at every call site, and adds tests covering: app.config.errorHandler consumer, onErrorCaptured consumer, single-emit semantics across multiple mismatches, and the no-consumer path. Signed-off-by: Pierluigi Lenoci <pierluigilenoci@gmail.com>
68656d1 to
1371d41
Compare
|
@pierluigilenoci @edison1105 Thanks for working on this. Any updates on your side that could help unblock it? Happy to help if needed. |
Keep the original production behavior unchanged: apps without an
explicit error consumer (errorHandler / onErrorCaptured) still see
the console.error('Hydration completed but contains mismatches.')
output. Only route through handleError when a consumer is present.
This addresses reviewer feedback that the previous revision removed
the default fallback, changing semantics for apps that had not opted in.
Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
|
@edison1105 thank you for the feedback — you're right that the previous revision dropped the production I just pushed a fix that preserves the original default behavior exactly:
The change is minimal: a single Let me know if this shape works for you. |
Summary
Hydration mismatch errors were never routed through Vue's error handling pipeline, so
onErrorCapturedandapp.config.errorHandlercould not catch them. This made it impossible to wire hydration-mismatch reporting into observability tools (Sentry, Datadog, etc.) — see issue #13154.This PR routes hydration mismatches through
handleError, but only when an explicit consumer is present, so SSR apps that have not opted in see no behavior change.Behavior
logMismatchErrorcallshandleError(..., ErrorCodes.HYDRATION_MISMATCH, false)only when one of the following is true on the parent component instance:instance.appContext.config.errorHandleris set, oronErrorCaptured.Without a consumer, the per-mismatch
warn()calls at the call sites remain the only output — matching the prior default behavior. No "Unhandled error during execution of hydration" warning is emitted for SSR apps that have not opted in.Changes
packages/runtime-core/src/errorHandling.ts: addsErrorCodes.HYDRATION_MISMATCHand its'hydration'info label.packages/runtime-core/src/hydration.ts:logMismatchErrornow takes the parent component instance, gateshandleErroron consumer presence (errorHandleror any ancestor withonErrorCaptured), and is invoked withparentComponentat every mismatch call site.packages/runtime-core/__tests__/hydration.spec.ts: adds 4 tests covering:app.config.errorHandlerconsumer,onErrorCapturedconsumer, single-emit semantics across multiple mismatches, and the no-consumer path (asserts the unhandled-error warning is not emitted).Reviewer feedback addressed
@edison1105 raised concerns about semantics changes for apps that had already configured
errorHandler/onErrorCaptured, and about the unhandled-error warning fallback for default apps. The current revision keeps default behavior unchanged and only opens the integration to apps that have explicitly opted in by registering a consumer.Test plan
pnpm vitest run packages/runtime-core/__tests__/hydration.spec.ts— 108 tests passpnpm lint— cleanpnpm check(tsc) — cleanmainCloses #13154.
Summary by CodeRabbit
Release Notes
Bug Fixes
errorHandlerand component-levelonErrorCapturedcallbacks when present.Tests